-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: api integration tests #280
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## daan/api #280 +/- ##
=========================================
Coverage 45.92% 45.92%
=========================================
Files 47 47
Lines 4279 4279
Branches 4279 4279
=========================================
Hits 1965 1965
Misses 2252 2252
Partials 62 62 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor! I'm going to approve, but please address Tin's comments as I agree with them
Err(Module { index: 52, error: [16, 0] }), | ||
); | ||
thaw_asset(addr.clone(), asset); | ||
pallet_assets_thaw(&addr, token); | ||
// TODO: calling the below with a vector of length `100_000` errors in pallet contracts | ||
// `OutputBufferTooSmall. Added to security analysis issue #131 to revisit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is an issue, just means pallet-contracts properly errors with values too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to keep in mind though because whenever the developer wants to set metadata and uses a value that is too large. In stead of pallet assets that returns an error it is pallet contracts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would expect that it might fail with out of gas realistically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some very minor improvements suggested, wouldnt be against merging as is however.
Err(Module { index: 52, error: [16, 0] }), | ||
); | ||
thaw_asset(addr.clone(), asset); | ||
pallet_assets_thaw(&addr, token); | ||
// TODO: calling the below with a vector of length `100_000` errors in pallet contracts | ||
// `OutputBufferTooSmall. Added to security analysis issue #131 to revisit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would expect that it might fail with out of gas realistically.
Contains all changes coming from comments for
pop-api
in #132 . Please not that in the comments I used #279 (incorrect) and #280. They both count for this PR.Changes include: